registry: per-identity token rotation via token_min_iat (fixes #2205) - #2208
registry: per-identity token rotation via token_min_iat (fixes #2205)#2208hognek wants to merge 4 commits into
Conversation
|
|
📝 WalkthroughWalkthroughThe change adds per-identity ChangesToken rotation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant rotate_tokens
participant AgentRegistryStore
participant GovernanceAudit
Caller->>rotate_tokens: POST /api/agents/registry/{canonical_id}/rotate-tokens
rotate_tokens->>AgentRegistryStore: load record and authorize owner or admin
rotate_tokens->>AgentRegistryStore: bump_token_min_iat(canonical_id, current timestamp)
rotate_tokens->>GovernanceAudit: record before and after cutoff values
rotate_tokens-->>Caller: updated registry record
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoRegistry: per-identity token rotation via token_min_iat
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| return { | ||
| "status": "approved", | ||
| "invite_id": invite["id"], |
There was a problem hiding this comment.
CRITICAL: KeyError — invite["id"] should be invite["record"]["invite_id"]
invite_store.mint() returns {"record": {..., "invite_id": ...}, "pin": ...}. Accessing invite["id"] raises an unhandled KeyError, causing a 500 whenever auto-approve is enabled.
| "invite_id": invite["id"], | |
| "invite_id": invite["record"]["invite_id"], |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| return { | ||
| "status": "approved", | ||
| "invite_id": invite["id"], |
There was a problem hiding this comment.
CRITICAL: KeyError — invite["id"] should be invite["record"]["invite_id"]
Same typo in complete_delegation_approval. invite_store.mint() returns a dict with record.invite_id, not a top-level id key. This crashes the approval flow when a delegation decision is approved.
| "invite_id": invite["id"], | |
| invite_id = invite["record"]["invite_id"], |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tinyagentos/routes/peer.py (1)
258-273: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDispatch branch sits below the "unrecognised kind" log.
Every
delegation_requestnow logsunrecognised kind — accepted, no dispatchbefore being dispatched, which will mislead anyone debugging the peer channel. Move the branch above the log (and drop the duplicatebody_dataread from Line 250).♻️ Proposed fix
+ # Dispatch delegation requests to the cross-user collab handler (D1). + if kind == "delegation_request": + from tinyagentos.delegation_handler import process_delegation_request + + result = await process_delegation_request( + request, contact_id=contact_id, envelope_body=body_data, + ) + return {"status": result.get("status", "unknown"), "detail": result} + # Log unrecognised kinds for debugging; they are accepted but not dispatched. logger.info( "peer_inbox: contact=%s kind=%s nonce=%s (unrecognised kind — accepted, no dispatch)", contact_id, kind, envelope.get("nonce", "?"), ) - # Dispatch delegation requests to the cross-user collab handler (D1). - if kind == "delegation_request": - from tinyagentos.delegation_handler import process_delegation_request - - body_data = envelope.get("body", {}) - result = await process_delegation_request( - request, contact_id=contact_id, envelope_body=body_data, - ) - return {"status": result.get("status", "unknown"), "detail": result} - return {"status": "received", "kind": kind, "nonce": envelope.get("nonce")}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/peer.py` around lines 258 - 273, Move the delegation_request dispatch branch in the peer message handler above the unrecognised-kind logger so recognized requests are not logged as unrecognised. Reuse the existing body_data value from the earlier code instead of reading envelope.get("body", {}) again, while preserving the process_delegation_request call and response handling.
🧹 Nitpick comments (10)
tinyagentos/routes/decisions.py (1)
476-479: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the extraneous
fprefix.♻️ Proposed fix
- error_msg = ( - f"Delegation approval failed: an internal error occurred while " - f"setting up the sponsored agent. Please retry or check the logs." - ) + error_msg = ( + "Delegation approval failed: an internal error occurred while " + "setting up the sponsored agent. Please retry or check the logs." + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/decisions.py` around lines 476 - 479, Remove the unnecessary f-string prefix from the error_msg assignment in the delegation approval error-handling code, since the message contains no interpolated expressions. Preserve the existing message text unchanged.Source: Linters/SAST tools
tinyagentos/projects/project_store.py (1)
382-404: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicated branches into one query.
♻️ Optional simplification
- if member_kind is not None: - async with self._db.execute( - "SELECT 1 FROM project_members " - "WHERE project_id = ? AND member_id = ? AND member_kind = ?", - (project_id, member_id, member_kind), - ) as cur: - return (await cur.fetchone()) is not None - else: - async with self._db.execute( - "SELECT 1 FROM project_members " - "WHERE project_id = ? AND member_id = ?", - (project_id, member_id), - ) as cur: - return (await cur.fetchone()) is not None + sql = "SELECT 1 FROM project_members WHERE project_id = ? AND member_id = ?" + params: list = [project_id, member_id] + if member_kind is not None: + sql += " AND member_kind = ?" + params.append(member_kind) + async with self._db.execute(sql, params) as cur: + return (await cur.fetchone()) is not None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/projects/project_store.py` around lines 382 - 404, Update is_project_member to use a single SQL query and parameter set, making the member_kind predicate optional while preserving filtering by member_kind when provided and matching any kind when omitted. Remove the duplicated execute branches and retain the existing boolean result behavior.tinyagentos/delegation_handler.py (2)
563-589: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftKill switch is process-local and not persisted.
app.state._peer_disabledresets on restart and is not shared across workers, so a level-3 panic silently lapses (or applies to only one worker under multi-process deployment). Consider persisting the flag (settings/DB) and reading it inis_peer_disabled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/delegation_handler.py` around lines 563 - 589, The level-3 peer disable state currently exists only in process-local app state. Update kill_switch_per_instance, kill_switch_reenable, and is_peer_disabled to persist the flag through the existing shared settings or database mechanism, and have is_peer_disabled read that persisted value so the panic survives restarts and is consistent across workers.
500-522: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent swallow makes partial unassign undebuggable.
Line 521-522 drops every per-task failure with no log, so
unassigned_countunder-reports with no trace — exactly the forensic trail a kill-switch needs. Alsoexcept AttributeErroronly guards the call itself; an awaited coroutine raisingAttributeErrorinternally would be misrouted to the fallback.♻️ Proposed fix
try: await task_store.update_task(task_id, assignee_id=None, status="ready") count += 1 - except Exception: - pass + except Exception: + logger.warning( + "cascade_revoke: failed to unassign task %s from %s", + task_id, canonical_id, exc_info=True, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/delegation_handler.py` around lines 500 - 522, Update the task-unassignment loop around task_store.update_task to log each per-task exception with the task identifier before continuing, preserving the count behavior for successful updates. Narrow the AttributeError handling around task_store.list_for_assignee so only a missing-method condition triggers the list_tasks fallback, while AttributeError raised during coroutine execution is propagated or handled as a real failure rather than misrouted.Source: Linters/SAST tools
tinyagentos/routes/project_invites.py (1)
1124-1131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract this metadata parsing (duplicated at Lines 1224-1231) and enforce a string sponsor id.
♻️ Proposed helper
def _sponsor_from_invite(invite: dict) -> str | None: meta = invite.get("metadata") or {} if isinstance(meta, str): try: meta = json.loads(meta) except (ValueError, TypeError): meta = {} if not isinstance(meta, dict): return None sponsor = meta.get("sponsor_contact_id") return sponsor if isinstance(sponsor, str) and sponsor.strip() else None- invite_metadata = invite.get("metadata") or {} - if isinstance(invite_metadata, str): - try: - invite_metadata = json.loads(invite_metadata) - except (ValueError, TypeError): - invite_metadata = {} - sponsor_contact_id = invite_metadata.get("sponsor_contact_id") if isinstance(invite_metadata, dict) else None + sponsor_contact_id = _sponsor_from_invite(invite)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/project_invites.py` around lines 1124 - 1131, The duplicated sponsor metadata parsing in the invite handling paths should be centralized and should only return valid non-empty string IDs. Add a helper such as _sponsor_from_invite, replace both parsing blocks around the existing extraction sites with calls to it, and preserve None for invalid, missing, non-dictionary, malformed, or blank sponsor_contact_id values.tests/test_collab_d1_delegation.py (1)
270-300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the delegation mint paths.
The store/validation layers are covered, but
_auto_approve_delegationandcomplete_delegation_approval(the code that actually reads themintresult and theproject_invite_storeapp-state attribute) have no test. Both contain contract bugs flagged intinyagentos/delegation_handler.pythat a single test would surface.Want me to draft those tests?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_collab_d1_delegation.py` around lines 270 - 300, The existing TestInviteMetadata coverage only exercises ProjectInviteStore.mint directly; add a test covering the delegation mint flow through _auto_approve_delegation and complete_delegation_approval. Configure the app state’s project_invite_store, invoke the delegation approval path, and assert it correctly consumes the mint result and completes approval without contract errors, including the expected invite/approval outcome.tinyagentos/agent_registry_store.py (1)
845-863: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
bump_token_min_iatcan lower the cutoff, silently un-superseding old tokens.The UPDATE unconditionally sets
token_min_iat = ?regardless of the current value. If this is ever called with a timestamp earlier than the existing cutoff (e.g., clock skew, a stale/duplicate rotation call, or a future caller other than the route), tokens that were already superseded become valid again — defeating the security guarantee this feature exists to provide.🔒 Proposed fix: make the cutoff monotonically non-decreasing
await self._db.execute( - "UPDATE agent_registry SET token_min_iat = ? WHERE canonical_id = ?", + "UPDATE agent_registry SET token_min_iat = MAX(token_min_iat, ?) WHERE canonical_id = ?", (ts, canonical_id), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/agent_registry_store.py` around lines 845 - 863, Update bump_token_min_iat so the database update only raises token_min_iat, never lowers an existing cutoff; use a monotonic SQL condition or equivalent comparison while preserving the existing missing-record and updated-record behavior.tests/test_token_rotation.py (2)
388-403: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
tokenbindings.Static analysis flags
tokenas unpacked-but-unused in bothtest_admin_can_rotate(Line 390) andtest_owner_can_rotate(Line 399).🧹 Proposed fix
- cid, token = await _register_and_mint(app, user_id="admin") + cid, _token = await _register_and_mint(app, user_id="admin")(apply to both occurrences)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_token_rotation.py` around lines 388 - 403, Remove the unused token bindings in both test_admin_can_rotate and test_owner_can_rotate by unpacking only the cid value returned from _register_and_mint, while preserving the existing rotation requests and assertions.Source: Linters/SAST tools
22-236: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftLarge duplicated fixture instead of reusing the existing one.
The docstring states this fixture should Reuse the same init + admin-session logic as the main client fixture, but the ~200-line store-init/close sequence appears to be copy-pasted into this new file rather than depending on the existing fixture. If an equivalent fixture (e.g., the one backing the main API test client) already exists in
conftest.py, depending on it directly and layering only the two extraagent_registry/agent_grantsinits on top would avoid this duplication and its maintenance burden (every new store added to the app must now be duplicated here too).#!/bin/bash # Locate the fixture this docstring claims to mirror, to confirm duplication. fd conftest.py --type f rg -n "async def .*client" tests/conftest.py -A5 2>/dev/null | head -50 rg -n "def app\(" tests/conftest.py -A5 2>/dev/null | head -50🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_token_rotation.py` around lines 22 - 236, Refactor the agent_app fixture to depend on the existing main client fixture from conftest.py, preserving its initialized stores, admin session, and cleanup behavior. Remove the duplicated store initialization and teardown sequence, and retain only the agent_registry and agent_grants initialization needed by this fixture before yielding the reused client.tinyagentos/routes/agent_registry.py (1)
770-777: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAudit entry carries no information about the actual rotation.
_audit_governanceis called withbefore_status/after_statusboth derived fromstatus(unaffected by rotation), so the persisted audit record always showsbefore == afterand doesn't capture what changed (the old vs. newtoken_min_iat). This weakens the "forensic audit-log entry" the PR objective calls for — an investigator can see that a rotation happened and who triggered it, but not the cutoff values involved.Consider extending the audit payload (or
_audit_governance) to includerecord["token_min_iat"](before) andts(after) instead of reusing the status fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/agent_registry.py` around lines 770 - 777, Update the rotate-tokens audit flow around _audit_governance to record the actual cutoff transition: use the pre-rotation record["token_min_iat"] as the before value and ts as the after value, rather than deriving both from status. Extend the audit payload or _audit_governance as needed while preserving the actor and rotation context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tinyagentos/delegation_handler.py`:
- Around line 294-298: Update both _auto_approve_delegation
(tinyagentos/delegation_handler.py#L294-L298) and complete_delegation_approval
(tinyagentos/delegation_handler.py#L378-L382) to read the minted invite
identifier from invite["record"]["invite_id"] instead of invite["id"],
preserving the existing response shape.
- Around line 264-266: Update the invite-store lookups in both
_auto_approve_delegation and complete_delegation_approval to read
request.app.state.project_invites instead of project_invite_store. Preserve the
existing unavailable-store error response and all surrounding delegation
behavior.
- Around line 416-420: Update the project-scoped filtering around project_id and
project_store so a set project_id without a project_store errors out immediately
instead of skipping membership checks. Preserve the existing is_project_member
filtering when project_store is available, preventing revocation of identities
outside the requested project.
- Around line 154-171: Restrict the auto-approval path in the delegation handler
around validate_delegation_scopes and _auto_approve_delegation to grant only
scopes in SPONSORED_DEFAULT_SCOPES; scopes outside that default tier must not be
auto-approved and must continue through explicit per-scope Decisions approval,
while preserving the existing hard-denial behavior.
In `@tinyagentos/projects/project_store.py`:
- Around line 405-417: Update get_project_setting to validate that the project’s
settings value is a dictionary before calling get on it. Return default when
settings is missing or any non-dict value, while preserving keyed lookup
behavior for valid dictionaries and matching the guard used by
set_project_setting.
In `@tinyagentos/routes/agent_auth_requests.py`:
- Around line 460-463: Update the existing-identity reuse branch around
registry.set_sponsor so sponsorship is assigned only when the identity currently
has no sponsor. Load or reuse the existing identity’s current sponsor value,
preserve any existing or different sponsor, and skip set_sponsor when it is
already populated.
In `@tinyagentos/routes/agent_registry.py`:
- Around line 761-778: Handle a None result from store.bump_token_min_iat in the
token-rotation flow before accessing updated.get or auditing; return the same
404 not-found response used when the initial store.get returns None, while
preserving the existing success path for a returned record.
---
Outside diff comments:
In `@tinyagentos/routes/peer.py`:
- Around line 258-273: Move the delegation_request dispatch branch in the peer
message handler above the unrecognised-kind logger so recognized requests are
not logged as unrecognised. Reuse the existing body_data value from the earlier
code instead of reading envelope.get("body", {}) again, while preserving the
process_delegation_request call and response handling.
---
Nitpick comments:
In `@tests/test_collab_d1_delegation.py`:
- Around line 270-300: The existing TestInviteMetadata coverage only exercises
ProjectInviteStore.mint directly; add a test covering the delegation mint flow
through _auto_approve_delegation and complete_delegation_approval. Configure the
app state’s project_invite_store, invoke the delegation approval path, and
assert it correctly consumes the mint result and completes approval without
contract errors, including the expected invite/approval outcome.
In `@tests/test_token_rotation.py`:
- Around line 388-403: Remove the unused token bindings in both
test_admin_can_rotate and test_owner_can_rotate by unpacking only the cid value
returned from _register_and_mint, while preserving the existing rotation
requests and assertions.
- Around line 22-236: Refactor the agent_app fixture to depend on the existing
main client fixture from conftest.py, preserving its initialized stores, admin
session, and cleanup behavior. Remove the duplicated store initialization and
teardown sequence, and retain only the agent_registry and agent_grants
initialization needed by this fixture before yielding the reused client.
In `@tinyagentos/agent_registry_store.py`:
- Around line 845-863: Update bump_token_min_iat so the database update only
raises token_min_iat, never lowers an existing cutoff; use a monotonic SQL
condition or equivalent comparison while preserving the existing missing-record
and updated-record behavior.
In `@tinyagentos/delegation_handler.py`:
- Around line 563-589: The level-3 peer disable state currently exists only in
process-local app state. Update kill_switch_per_instance, kill_switch_reenable,
and is_peer_disabled to persist the flag through the existing shared settings or
database mechanism, and have is_peer_disabled read that persisted value so the
panic survives restarts and is consistent across workers.
- Around line 500-522: Update the task-unassignment loop around
task_store.update_task to log each per-task exception with the task identifier
before continuing, preserving the count behavior for successful updates. Narrow
the AttributeError handling around task_store.list_for_assignee so only a
missing-method condition triggers the list_tasks fallback, while AttributeError
raised during coroutine execution is propagated or handled as a real failure
rather than misrouted.
In `@tinyagentos/projects/project_store.py`:
- Around line 382-404: Update is_project_member to use a single SQL query and
parameter set, making the member_kind predicate optional while preserving
filtering by member_kind when provided and matching any kind when omitted.
Remove the duplicated execute branches and retain the existing boolean result
behavior.
In `@tinyagentos/routes/agent_registry.py`:
- Around line 770-777: Update the rotate-tokens audit flow around
_audit_governance to record the actual cutoff transition: use the pre-rotation
record["token_min_iat"] as the before value and ts as the after value, rather
than deriving both from status. Extend the audit payload or _audit_governance as
needed while preserving the actor and rotation context.
In `@tinyagentos/routes/decisions.py`:
- Around line 476-479: Remove the unnecessary f-string prefix from the error_msg
assignment in the delegation approval error-handling code, since the message
contains no interpolated expressions. Preserve the existing message text
unchanged.
In `@tinyagentos/routes/project_invites.py`:
- Around line 1124-1131: The duplicated sponsor metadata parsing in the invite
handling paths should be centralized and should only return valid non-empty
string IDs. Add a helper such as _sponsor_from_invite, replace both parsing
blocks around the existing extraction sites with calls to it, and preserve None
for invalid, missing, non-dictionary, malformed, or blank sponsor_contact_id
values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 91988a57-fb4a-4d47-ba8c-18736de70c41
📒 Files selected for processing (13)
tests/test_agent_registry_store.pytests/test_collab_d1_delegation.pytests/test_token_rotation.pytinyagentos/agent_registry_store.pytinyagentos/agent_token_auth.pytinyagentos/delegation_handler.pytinyagentos/projects/invite_store.pytinyagentos/projects/project_store.pytinyagentos/routes/agent_auth_requests.pytinyagentos/routes/agent_registry.pytinyagentos/routes/decisions.pytinyagentos/routes/peer.pytinyagentos/routes/project_invites.py
| granted_scopes, denied_scopes = validate_delegation_scopes(requested_scopes) | ||
|
|
||
| # Check project policy: auto_approve_delegation knob. | ||
| auto_approve = await project_store.get_project_setting( | ||
| project_id, "auto_approve_delegation", default=False | ||
| ) | ||
|
|
||
| if auto_approve: | ||
| # Future dev-swarm path: immediate approval. | ||
| return await _auto_approve_delegation( | ||
| request, | ||
| contact_id=contact_id, | ||
| agent_slug=agent_slug, | ||
| display_name=display_name, | ||
| granted_scopes=granted_scopes, | ||
| denied_scopes=denied_scopes, | ||
| project_id=project_id, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Auto-approve mints every non-denied scope, contradicting the documented tier.
validate_delegation_scopes only strips the two hard-denied scopes, and its own docstring (Lines 48-50) states scopes outside SPONSORED_DEFAULT_SCOPES "require explicit per-scope Decisions approval". On the auto_approve_delegation path no human sees them, so a remote contact can self-grant e.g. canvas_write or files_read by listing them in the envelope.
🔒 Proposed fix — restrict the auto path to the default tier
if auto_approve:
# Future dev-swarm path: immediate approval.
+ auto_scopes = sorted(set(granted_scopes) & SPONSORED_DEFAULT_SCOPES)
+ elevated = sorted(set(granted_scopes) - SPONSORED_DEFAULT_SCOPES)
+ if elevated:
+ logger.warning(
+ "delegation: auto-approve dropped elevated scopes %r for %s",
+ elevated, contact_id,
+ )
return await _auto_approve_delegation(
request,
contact_id=contact_id,
agent_slug=agent_slug,
display_name=display_name,
- granted_scopes=granted_scopes,
- denied_scopes=denied_scopes,
+ granted_scopes=auto_scopes,
+ denied_scopes=denied_scopes + elevated,
project_id=project_id,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| granted_scopes, denied_scopes = validate_delegation_scopes(requested_scopes) | |
| # Check project policy: auto_approve_delegation knob. | |
| auto_approve = await project_store.get_project_setting( | |
| project_id, "auto_approve_delegation", default=False | |
| ) | |
| if auto_approve: | |
| # Future dev-swarm path: immediate approval. | |
| return await _auto_approve_delegation( | |
| request, | |
| contact_id=contact_id, | |
| agent_slug=agent_slug, | |
| display_name=display_name, | |
| granted_scopes=granted_scopes, | |
| denied_scopes=denied_scopes, | |
| project_id=project_id, | |
| ) | |
| granted_scopes, denied_scopes = validate_delegation_scopes(requested_scopes) | |
| # Check project policy: auto_approve_delegation knob. | |
| auto_approve = await project_store.get_project_setting( | |
| project_id, "auto_approve_delegation", default=False | |
| ) | |
| if auto_approve: | |
| # Future dev-swarm path: immediate approval. | |
| auto_scopes = sorted(set(granted_scopes) & SPONSORED_DEFAULT_SCOPES) | |
| elevated = sorted(set(granted_scopes) - SPONSORED_DEFAULT_SCOPES) | |
| if elevated: | |
| logger.warning( | |
| "delegation: auto-approve dropped elevated scopes %r for %s", | |
| elevated, contact_id, | |
| ) | |
| return await _auto_approve_delegation( | |
| request, | |
| contact_id=contact_id, | |
| agent_slug=agent_slug, | |
| display_name=display_name, | |
| granted_scopes=auto_scopes, | |
| denied_scopes=denied_scopes + elevated, | |
| project_id=project_id, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/delegation_handler.py` around lines 154 - 171, Restrict the
auto-approval path in the delegation handler around validate_delegation_scopes
and _auto_approve_delegation to grant only scopes in SPONSORED_DEFAULT_SCOPES;
scopes outside that default tier must not be auto-approved and must continue
through explicit per-scope Decisions approval, while preserving the existing
hard-denial behavior.
| invite_store = getattr(request.app.state, "project_invite_store", None) | ||
| if invite_store is None: | ||
| return {"status": "error", "error": "invite store not available"} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Wrong app.state attribute — the invite store is project_invites.
Everywhere else in the codebase the store is registered as request.app.state.project_invites (routes/peer.py Line 511, routes/project_invites.py Line 1092, and the test fixtures). project_invite_store is never set, so both _auto_approve_delegation and complete_delegation_approval (Line 340) always short-circuit with "invite store not available" — delegation can never complete.
🐛 Proposed fix (apply at both sites)
- invite_store = getattr(request.app.state, "project_invite_store", None)
+ invite_store = getattr(request.app.state, "project_invites", None)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| invite_store = getattr(request.app.state, "project_invite_store", None) | |
| if invite_store is None: | |
| return {"status": "error", "error": "invite store not available"} | |
| invite_store = getattr(request.app.state, "project_invites", None) | |
| if invite_store is None: | |
| return {"status": "error", "error": "invite store not available"} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/delegation_handler.py` around lines 264 - 266, Update the
invite-store lookups in both _auto_approve_delegation and
complete_delegation_approval to read request.app.state.project_invites instead
of project_invite_store. Preserve the existing unavailable-store error response
and all surrounding delegation behavior.
| return { | ||
| "status": "approved", | ||
| "invite_id": invite["id"], | ||
| "agent_slug": agent_slug, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
mint return-shape mismatch at both delegation mint sites. ProjectInviteStore.mint returns {"record": {..., "invite_id": ...}, "pin": ...}, so invite["id"] raises KeyError in both handlers; both reads sit outside the surrounding try, so the failure escapes as a 500 / generic "approval failed".
tinyagentos/delegation_handler.py#L294-L298: in_auto_approve_delegation, returninvite["record"]["invite_id"].tinyagentos/delegation_handler.py#L378-L382: incomplete_delegation_approval, returninvite["record"]["invite_id"].
📍 Affects 1 file
tinyagentos/delegation_handler.py#L294-L298(this comment)tinyagentos/delegation_handler.py#L378-L382
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/delegation_handler.py` around lines 294 - 298, Update both
_auto_approve_delegation (tinyagentos/delegation_handler.py#L294-L298) and
complete_delegation_approval (tinyagentos/delegation_handler.py#L378-L382) to
read the minted invite identifier from invite["record"]["invite_id"] instead of
invite["id"], preserving the existing response shape.
| if project_id is not None and project_store is not None: | ||
| if not await project_store.is_project_member( | ||
| project_id, canonical_id | ||
| ): | ||
| continue |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Project-scoped revoke fails open when project_store is missing.
With project_id set but no project_store on app.state, the membership filter is skipped and every sponsored identity for the contact is revoked — a wider blast radius than the caller asked for. Prefer erroring out instead.
🛡️ Proposed fix
+ if project_id is not None and project_store is None:
+ return {"status": "error", "error": "project store not available"}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if project_id is not None and project_store is not None: | |
| if not await project_store.is_project_member( | |
| project_id, canonical_id | |
| ): | |
| continue | |
| if project_id is not None and project_store is None: | |
| return {"status": "error", "error": "project store not available"} | |
| if project_id is not None and project_store is not None: | |
| if not await project_store.is_project_member( | |
| project_id, canonical_id | |
| ): | |
| continue |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/delegation_handler.py` around lines 416 - 420, Update the
project-scoped filtering around project_id and project_store so a set project_id
without a project_store errors out immediately instead of skipping membership
checks. Preserve the existing is_project_member filtering when project_store is
available, preventing revocation of identities outside the requested project.
| async def get_project_setting( | ||
| self, project_id: str, key: str, default=None | ||
| ): | ||
| """Read a single key from the project's JSON settings dict. | ||
|
|
||
| Returns *default* if the project does not exist, has no settings, or | ||
| the key is absent. | ||
| """ | ||
| project = await self.get_project(project_id) | ||
| if project is None: | ||
| return default | ||
| settings = project.get("settings") or {} | ||
| return settings.get(key, default) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against a non-dict settings here too.
set_project_setting defends with isinstance(settings, dict) (Line 430) but the reader does not; a legacy row whose settings JSON is a list/scalar makes this raise AttributeError instead of returning default.
🛡️ Proposed fix
- settings = project.get("settings") or {}
- return settings.get(key, default)
+ settings = project.get("settings") or {}
+ if not isinstance(settings, dict):
+ return default
+ return settings.get(key, default)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def get_project_setting( | |
| self, project_id: str, key: str, default=None | |
| ): | |
| """Read a single key from the project's JSON settings dict. | |
| Returns *default* if the project does not exist, has no settings, or | |
| the key is absent. | |
| """ | |
| project = await self.get_project(project_id) | |
| if project is None: | |
| return default | |
| settings = project.get("settings") or {} | |
| return settings.get(key, default) | |
| async def get_project_setting( | |
| self, project_id: str, key: str, default=None | |
| ): | |
| """Read a single key from the project's JSON settings dict. | |
| Returns *default* if the project does not exist, has no settings, or | |
| the key is absent. | |
| """ | |
| project = await self.get_project(project_id) | |
| if project is None: | |
| return default | |
| settings = project.get("settings") or {} | |
| if not isinstance(settings, dict): | |
| return default | |
| return settings.get(key, default) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/projects/project_store.py` around lines 405 - 417, Update
get_project_setting to validate that the project’s settings value is a
dictionary before calling get on it. Return default when settings is missing or
any non-dict value, while preserving keyed lookup behavior for valid
dictionaries and matching the guard used by set_project_setting.
| # For sponsored agents (cross-user collab D1), set the sponsor | ||
| # on the existing identity when reusing it for a new project. | ||
| if sponsor_contact_id: | ||
| await registry.set_sponsor(existing_cid, sponsor_contact_id) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reusing an existing handle can retro-assign sponsorship of a pre-existing identity.
This branch fires when the derived handle already maps to an ACTIVE identity that may have been created by a normal consent flow (or sponsored by a different contact). Setting sponsor_contact_id unconditionally brings that identity under the new contact's cascade_sponsor_revoke authority and overwrites a previous sponsor. Only set it when the row currently has no sponsor.
🔒 Proposed fix
- if sponsor_contact_id:
- await registry.set_sponsor(existing_cid, sponsor_contact_id)
+ if sponsor_contact_id and not existing_active.get("sponsor_contact_id"):
+ await registry.set_sponsor(existing_cid, sponsor_contact_id)
+ elif sponsor_contact_id and existing_active.get("sponsor_contact_id") != sponsor_contact_id:
+ logger.warning(
+ "auth-approve: refusing to re-sponsor %s (existing sponsor %r)",
+ existing_cid, existing_active.get("sponsor_contact_id"),
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # For sponsored agents (cross-user collab D1), set the sponsor | |
| # on the existing identity when reusing it for a new project. | |
| if sponsor_contact_id: | |
| await registry.set_sponsor(existing_cid, sponsor_contact_id) | |
| # For sponsored agents (cross-user collab D1), set the sponsor | |
| # on the existing identity when reusing it for a new project. | |
| if sponsor_contact_id and not existing_active.get("sponsor_contact_id"): | |
| await registry.set_sponsor(existing_cid, sponsor_contact_id) | |
| elif sponsor_contact_id and existing_active.get("sponsor_contact_id") != sponsor_contact_id: | |
| logger.warning( | |
| "auth-approve: refusing to re-sponsor %s (existing sponsor %r)", | |
| existing_cid, existing_active.get("sponsor_contact_id"), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/routes/agent_auth_requests.py` around lines 460 - 463, Update the
existing-identity reuse branch around registry.set_sponsor so sponsorship is
assigned only when the identity currently has no sponsor. Load or reuse the
existing identity’s current sponsor value, preserve any existing or different
sponsor, and skip set_sponsor when it is already populated.
| from tinyagentos.delegation_handler import process_delegation_request | ||
|
|
||
| body_data = envelope.get("body", {}) | ||
| result = await process_delegation_request( |
There was a problem hiding this comment.
WARNING: Misleading log — delegation_request is dispatched but logged as "unrecognised kind"
The logger.info block above marks the kind as unrecognised before the delegation_request dispatch below. This masks real unrecognised kinds in logs.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Previous Review Summary (commit bbbf0ea)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit bbbf0ea)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (13 files)
Reviewed by step-3.7-flash · Input: 95.4K · Output: 21.5K · Cached: 473.1K |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| """Agent delegation handler — cross-user collab D1. | ||
|
|
||
| Processes delegation-request envelopes from remote contacts, applies | ||
| scope denylist and policy gates, creates Decisions cards for manual | ||
| approval, and on approval mints project invites for the sponsored agent. | ||
|
|
||
| Also provides cascade revocation and kill-switch machinery. | ||
| """ |
There was a problem hiding this comment.
3. Em dashes in docstrings 📜 Skill insight ✧ Quality
New/modified docstrings and comments include Unicode em dashes (—). This violates the no-em-dashes rule for public-facing text and developer-visible strings.
Agent Prompt
## Issue description
The PR introduces em dash characters (`—`, U+2014) in docstrings/comments/strings, which are disallowed.
## Issue Context
Compliance requires using regular hyphens (`-`) or rephrasing instead of em dashes in any public-facing/developer-facing text.
## Fix Focus Areas
- tinyagentos/delegation_handler.py[1-23]
- tinyagentos/agent_token_auth.py[20-24]
- tinyagentos/routes/agent_registry.py[754-760]
- tests/test_collab_d1_delegation.py[1-1]
- tests/test_agent_registry_store.py[1188-1190]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| invite_store = getattr(request.app.state, "project_invite_store", None) | ||
| if invite_store is None: | ||
| return {"status": "error", "error": "invite store not available"} | ||
|
|
||
| # The project-scoped scopes require a non-null project_id for the JWT. | ||
| project_scopes = sorted(set(granted_scopes) & _PROJECT_SCOPES) | ||
| non_project_scopes = sorted(set(granted_scopes) - _PROJECT_SCOPES) | ||
|
|
||
| try: | ||
| invite = await invite_store.mint( | ||
| project_id=project_id, | ||
| scopes=non_project_scopes + project_scopes, | ||
| approval_mode="auto", | ||
| check_interval_secs=1800, | ||
| created_by=contact_id, | ||
| metadata={ | ||
| "kind": "delegation_sponsored", | ||
| "sponsor_contact_id": contact_id, | ||
| "agent_slug": agent_slug, | ||
| "display_name": display_name, | ||
| "pin_required": False, | ||
| }, | ||
| ) | ||
| except Exception: | ||
| logger.warning( | ||
| "delegation: failed to create invite for %s / %s", | ||
| contact_id, agent_slug, exc_info=True, | ||
| ) | ||
| return {"status": "error", "error": "failed to create project invite"} | ||
|
|
||
| return { | ||
| "status": "approved", | ||
| "invite_id": invite["id"], | ||
| "agent_slug": agent_slug, | ||
| } |
There was a problem hiding this comment.
5. Delegation invite mint broken 🐞 Bug ≡ Correctness
delegation_handler looks up the invite store at app.state.project_invite_store and returns
invite['id'], but the app wires the store as app.state.project_invites and
ProjectInviteStore.mint() returns {record:{invite_id,...}, pin}. As a result, delegation
auto-approve and approved Decisions side-effects will fail at runtime (either always “invite store
not available” or raising KeyError).
Agent Prompt
### Issue description
`_auto_approve_delegation()` / `complete_delegation_approval()` are incompatible with the actual app wiring and `ProjectInviteStore.mint()` return shape.
### Issue Context
- The app sets the invite store on `app.state.project_invites`.
- `ProjectInviteStore.mint()` returns a dict with the invite id at `result["record"]["invite_id"]`.
### Fix Focus Areas
- tinyagentos/delegation_handler.py[249-299]
- tinyagentos/delegation_handler.py[340-383]
### Suggested change
- Replace `getattr(request.app.state, "project_invite_store", None)` with `getattr(request.app.state, "project_invites", None)`.
- When minting, treat the return value as `mint_result` and return `mint_result["record"]["invite_id"]` (and optionally return the `pin` if the flow needs it).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| requested_set = set(requested_scopes) | ||
| denied = sorted(requested_set & SPONSORED_DENY_SCOPES) | ||
| allowed = sorted(requested_set - SPONSORED_DENY_SCOPES) | ||
| if denied: |
There was a problem hiding this comment.
6. Delegation scopes can crash 🐞 Bug ☼ Reliability
validate_delegation_scopes() / _validate_delegation_envelope_body() build set(requested_scopes) without validating that every element is a string, so a peer can send an unhashable element (e.g. an object) and trigger a TypeError -> 500. This is a new externally reachable crash path via /api/peer/inbox delegation dispatch.
Agent Prompt
### Issue description
Delegation request validation checks only that `requested_scopes` is a list, but it does not validate list *elements* before using `set(...)`. Malformed JSON can trigger `TypeError: unhashable type` and crash the request.
### Issue Context
`/api/peer/inbox` dispatches `kind == "delegation_request"` directly into `process_delegation_request()`, so this error is reachable from authenticated peer traffic.
### Fix Focus Areas
- tinyagentos/delegation_handler.py[42-65]
- tinyagentos/delegation_handler.py[67-105]
- tinyagentos/routes/peer.py[264-273]
### Suggested change
- In `_validate_delegation_envelope_body`, validate that every entry in `requested_scopes` is a non-empty `str` (e.g., `all(isinstance(s, str) and s.strip() for s in value)`), otherwise return `(False, "requested_scopes must be a list of non-empty strings", None)`.
- Optionally defensively wrap `validate_delegation_scopes`’s `set(...)` conversion in a try/except and return a structured error if validation is bypassed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # Reject tokens issued before the identity's token_min_iat cutoff (rotation). | ||
| token_min_iat = record.get("token_min_iat") or 0 | ||
| token_iat = payload.get("iat") or 0 | ||
| if token_iat < token_min_iat: | ||
| raise HTTPException(status_code=401, detail="token superseded") |
There was a problem hiding this comment.
7. Superseded token still authorizes 🐞 Bug ⛨ Security
The new token_min_iat cutoff is enforced only inside _verify_agent_scope, but check_agent_identity() (used to authorize the scope-request creation flow) does not enforce token_min_iat. After rotating tokens, an old token can still authenticate scope-request creation for that identity.
Agent Prompt
### Issue description
Token rotation is implemented in `_verify_agent_scope()`, but other token verification paths still accept superseded tokens.
### Issue Context
`create_scope_request` authorizes via `check_agent_identity()` for agent self-auth, and `check_agent_identity()` currently does not check `token_min_iat`.
### Fix Focus Areas
- tinyagentos/agent_token_auth.py[83-120]
- tinyagentos/agent_token_auth.py[159-198]
- tinyagentos/routes/agent_auth_requests.py[905-929]
### Suggested change
- Add the same `token_min_iat` vs `payload["iat"]` check to `check_agent_identity()`.
- Preferably refactor into a shared helper that:
- verifies signature
- loads registry record
- checks status == active
- enforces token_min_iat cutoff
and is used by both `_verify_agent_scope()` and `check_agent_identity()` to prevent future drift.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ts = int(time.time()) | ||
| updated = await store.bump_token_min_iat(canonical_id, ts) |
There was a problem hiding this comment.
8. Same-second rotation loophole 🐞 Bug ⛨ Security
rotate-tokens stores token_min_iat = int(time.time()) and tokens are minted with `iat = int(time.time()); with the strict <` comparison, tokens minted earlier in the same second as the bump have iat == token_min_iat and remain valid. Rotation therefore does not reliably invalidate existing tokens without waiting for the next second boundary.
Agent Prompt
### Issue description
Token rotation compares second-resolution timestamps and uses a strict `<` comparison, leaving a same-second window where “old” tokens are not invalidated.
### Issue Context
- `mint_registry_token()` sets `iat` as `int(time.time())`.
- `rotate_tokens()` bumps `token_min_iat` to `int(time.time())`.
- Auth rejects only when `token_iat < token_min_iat`.
### Fix Focus Areas
- tinyagentos/routes/agent_registry.py[748-778]
- tinyagentos/agent_token_auth.py[115-120]
- tinyagentos/agent_registry_store.py[322-369]
### Suggested change (one viable approach)
- Switch `iat` and `token_min_iat` to higher precision values:
- set `iat` to `time.time()` (float seconds)
- set rotation cutoff `ts` to `time.time()` (float seconds)
- keep the strict `<` comparison
This avoids same-second collisions while keeping NumericDate semantics (seconds since epoch).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
fae0522 to
ee7a0a5
Compare
|
Heads up: this went conflicting today because dev just gained changes in agent_registry_store.py (a new get_by_slug with strict input gating, merged via #2225). When you rebase for the invite-shape criticals the bots flagged, take dev's version of that region and re-apply your token_min_iat changes on top. No rush from my side, flagging so the conflict source is not a mystery. |
ee7a0a5 to
e614763
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/test_token_rotation.py (2)
22-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffReuse the shared client fixture instead of copying it.
The
agent_appfixture duplicates about 200 lines of store init and teardown from the main client fixture. Any new store added toapp.statemust then be updated in two places, and the copy will silently drift. Buildagent_appon top of the existing client fixture and only add theagent_registry/agent_grantsinit that is specific to this file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_token_rotation.py` around lines 22 - 236, Refactor the agent_app fixture to depend on and reuse the existing shared client fixture instead of duplicating store initialization and teardown. Preserve only the agent_registry and agent_grants initialization specific to this fixture, and yield the shared client after that setup without adding parallel cleanup logic.
254-273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify that
user_iddoes not set the record owner.
_register_and_mintpassesuser_idonly tomint_registry_token.registry.register(...)receives nouser_id, sorecord["user_id"]in the route is unrelated to this argument. Any test that wants to exercise the owner branch ofrequire_owner_or_admincannot do it through this helper. Add an explicit owner argument that is forwarded toregister, or rename the parameter totoken_user_id.Also, the three tests in
TestTokenMinIatAuthrepeat the register/set_status/add_grant sequence instead of calling this helper. Route the tests through the helper to remove the duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_token_rotation.py` around lines 254 - 273, Clarify the ownership semantics of _register_and_mint by renaming its user_id parameter to token_user_id, or add a separate owner argument and forward it to registry.register; ensure owner-branch tests can explicitly set the registered record owner. Update the three TestTokenMinIatAuth tests to use _register_and_mint instead of duplicating registration, activation, and grant setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_token_rotation.py`:
- Around line 384-411: The TestRotateTokensRoute authorization tests currently
reuse the admin session and do not exercise owner or unauthorized access. Update
test_owner_can_rotate to create a distinct non-admin user/session and rotate an
identity owned by that user, then add a test using a different non-admin caller
that asserts a 403 response for the same endpoint; retain the existing
admin-success and nonexistent-identity coverage.
In `@tinyagentos/agent_registry_store.py`:
- Line 440: Update the registry migration chain around
_migration_v6_add_token_min_iat by implementing the missing _migration_v5_*
migration and invoking it in the correct version order. Extend the registry
lookup contract beyond get_by_handle by adding get_by_slug and updating related
registry interfaces or implementations consistently with existing patterns.
- Around line 769-786: Update the route’s audit entry to set after_token_min_iat
from the updated record’s token_min_iat returned by bump_token_min_iat, rather
than the requested ts, so the persisted cutoff is recorded. Preserve the
existing handling for a missing record; only add serialized before/after capture
if the implementation requires exact concurrent rotation pairs.
In `@tinyagentos/routes/agent_registry.py`:
- Around line 771-786: Update the token-rotation flow around bump_token_min_iat
and _audit_governance to use the persisted token_min_iat from updated for
after_token_min_iat, and ensure the response body reports that same value
instead of the requested ts.
---
Nitpick comments:
In `@tests/test_token_rotation.py`:
- Around line 22-236: Refactor the agent_app fixture to depend on and reuse the
existing shared client fixture instead of duplicating store initialization and
teardown. Preserve only the agent_registry and agent_grants initialization
specific to this fixture, and yield the shared client after that setup without
adding parallel cleanup logic.
- Around line 254-273: Clarify the ownership semantics of _register_and_mint by
renaming its user_id parameter to token_user_id, or add a separate owner
argument and forward it to registry.register; ensure owner-branch tests can
explicitly set the registered record owner. Update the three TestTokenMinIatAuth
tests to use _register_and_mint instead of duplicating registration, activation,
and grant setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fb701942-d558-4dee-adb1-a29b484345ba
📒 Files selected for processing (5)
tests/test_agent_registry_store.pytests/test_token_rotation.pytinyagentos/agent_registry_store.pytinyagentos/agent_token_auth.pytinyagentos/routes/agent_registry.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tinyagentos/agent_token_auth.py
- tests/test_agent_registry_store.py
| class TestRotateTokensRoute: | ||
| """Test POST /api/agents/registry/{id}/rotate-tokens via the admin client.""" | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_admin_can_rotate(self, agent_app, app): | ||
| """An admin can bump token_min_iat on any identity.""" | ||
| cid, _token = await _register_and_mint(app, user_id="admin") | ||
| resp = await agent_app.post(f"/api/agents/registry/{cid}/rotate-tokens") | ||
| assert resp.status_code == 200 | ||
| body = resp.json() | ||
| assert body["token_min_iat"] > 0 | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_owner_can_rotate(self, agent_app, app): | ||
| """A session owner (non-admin) can rotate their own identity.""" | ||
| cid, _token = await _register_and_mint(app, user_id="admin") | ||
| resp = await agent_app.post(f"/api/agents/registry/{cid}/rotate-tokens") | ||
| assert resp.status_code == 200 | ||
| body = resp.json() | ||
| assert body["token_min_iat"] > 0 | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_rotate_nonexistent_returns_404(self, agent_app): | ||
| """Rotating a nonexistent identity returns 404.""" | ||
| resp = await agent_app.post( | ||
| "/api/agents/registry/no-such-agent-20260101-000000/rotate-tokens" | ||
| ) | ||
| assert resp.status_code == 404 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
test_owner_can_rotate duplicates the admin test and leaves authorization untested.
test_owner_can_rotate uses the same agent_app client, which holds an admin session. Its body is identical to test_admin_can_rotate. The docstring claims a non-admin owner, but no non-admin session is created, so require_owner_or_admin is always satisfied through the admin branch. Two gaps remain:
- The owner branch (
user.user_id == record["user_id"]) is never executed. - No test asserts
403for a caller who is neither owner nor admin.
The linked issue lists authorization restrictions as a required verification. Create a second non-admin user, register an identity owned by that user, and add both an owner-success case and a non-owner 403 case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_token_rotation.py` around lines 384 - 411, The
TestRotateTokensRoute authorization tests currently reuse the admin session and
do not exercise owner or unauthorized access. Update test_owner_can_rotate to
create a distinct non-admin user/session and rotate an identity owned by that
user, then add a test using a different non-admin caller that asserts a 403
response for the same endpoint; retain the existing admin-success and
nonexistent-identity coverage.
| # Dedupe BEFORE the index so a pre-invariant DB with duplicate active | ||
| # handles cannot make the CREATE UNIQUE INDEX (hence boot) fail. | ||
| await _migration_v4_dedupe_active_handles(self._db) | ||
| await _migration_v6_add_token_min_iat(self._db) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
store="tinyagentos/agent_registry_store.py"
echo "Migration definitions and calls:"
rg -n -P '^\s*(async\s+)?def _migration_v[0-9]+\b|^\s*await _migration_v[0-9]+\(' "$store" || true
echo "Registry lookup implementations and callers:"
rg -n -P '\bget_by_slug\s*\(|\bget_by_handle\s*\(' . --glob '*.py' || trueRepository: jaylfc/taOS
Length of output: 1329
🏁 Script executed:
#!/bin/bash
set -euo pipefail
store="tinyagentos/agent_registry_store.py"
echo "Candidate migration/lookup references:"
rg -n '_migration_v[0-9]+|get_by_slug|get_by_handle|version|schema_version|CREATE TABLE|ALTER TABLE|CREATE INDEX|DROP TABLE' "$store" || true
echo
echo "Store class outline:"
ast-grep outline "$store" --match AgentRegistryStore --view expanded || true
echo
echo "Relevant source ranges:"
sed -n '400,455p' "$store"
printf '\n---\n'
sed -n '388,585p' "$store"
printf '\n---\n'
sed -n '585,695p' "$store"Repository: jaylfc/taOS
Length of output: 18305
Add the missing registry migration and lookup contract.
tinyagentos/agent_registry_store.py is missing _migration_v5_*, calls _migration_v6_add_token_min_iat without any migration in between, and still only exposes get_by_handle. Add the required v5 migration implementation and any missing get_by_slug or related registry contract updates before merge.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/agent_registry_store.py` at line 440, Update the registry
migration chain around _migration_v6_add_token_min_iat by implementing the
missing _migration_v5_* migration and invoking it in the correct version order.
Extend the registry lookup contract beyond get_by_handle by adding get_by_slug
and updating related registry interfaces or implementations consistently with
existing patterns.
| async def bump_token_min_iat(self, canonical_id: str, ts: int) -> Optional[dict]: | ||
| """Set *canonical_id*'s ``token_min_iat`` to *ts*, invalidating every | ||
| token minted before that Unix timestamp. | ||
|
|
||
| The caller is responsible for authorisation (admin/session-owner checks). | ||
| Returns the updated record, or ``None`` if *canonical_id* does not exist. | ||
| """ | ||
| if self._db is None: | ||
| raise RuntimeError("AgentRegistryStore not initialised") | ||
| record = await self.get(canonical_id) | ||
| if record is None: | ||
| return None | ||
| await self._db.execute( | ||
| "UPDATE agent_registry SET token_min_iat = MAX(token_min_iat, ?) WHERE canonical_id = ?", | ||
| (ts, canonical_id), | ||
| ) | ||
| await self._db.commit() | ||
| return await self.get(canonical_id) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Record the persisted cutoff in the audit entry.
Line [782] uses MAX(token_min_iat, ?), so the stored cutoff can be greater than ts. The route currently records after_token_min_iat=ts, which can make the forensic audit entry report a value that was not applied. Use updated["token_min_iat"]. If concurrent rotations must produce exact before/after pairs, capture both values inside one serialized store operation.
Minimum route fix
- after_token_min_iat=ts,
+ after_token_min_iat=updated["token_min_iat"],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/agent_registry_store.py` around lines 769 - 786, Update the
route’s audit entry to set after_token_min_iat from the updated record’s
token_min_iat returned by bump_token_min_iat, rather than the requested ts, so
the persisted cutoff is recorded. Preserve the existing handling for a missing
record; only add serialized before/after capture if the implementation requires
exact concurrent rotation pairs.
| ts = int(time.time()) | ||
| before_iat = record.get("token_min_iat") or 0 | ||
| updated = await store.bump_token_min_iat(canonical_id, ts) | ||
| if updated is None: | ||
| return JSONResponse({"error": "not found"}, status_code=404) | ||
|
|
||
| await _audit_governance( | ||
| request, | ||
| action="rotate-tokens", | ||
| canonical_id=canonical_id, | ||
| actor_user_id=user.user_id, | ||
| before_status=record.get("status") or "active", | ||
| after_status=updated.get("status") or "active", | ||
| before_token_min_iat=before_iat, | ||
| after_token_min_iat=ts, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Record the persisted cutoff, not the requested one.
bump_token_min_iat writes MAX(token_min_iat, ?). If the stored token_min_iat is already greater than ts (a previous bump under clock skew, or a manually seeded future cutoff), the database keeps the larger value. The audit event then reports after_token_min_iat=ts, which does not match the persisted row, and the response body reports a different value than the audit entry. Read the value back from updated.
🛠️ Proposed fix
updated = await store.bump_token_min_iat(canonical_id, ts)
if updated is None:
return JSONResponse({"error": "not found"}, status_code=404)
+ after_iat = updated.get("token_min_iat") or ts
await _audit_governance(
request,
action="rotate-tokens",
canonical_id=canonical_id,
actor_user_id=user.user_id,
before_status=record.get("status") or "active",
after_status=updated.get("status") or "active",
before_token_min_iat=before_iat,
- after_token_min_iat=ts,
+ after_token_min_iat=after_iat,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ts = int(time.time()) | |
| before_iat = record.get("token_min_iat") or 0 | |
| updated = await store.bump_token_min_iat(canonical_id, ts) | |
| if updated is None: | |
| return JSONResponse({"error": "not found"}, status_code=404) | |
| await _audit_governance( | |
| request, | |
| action="rotate-tokens", | |
| canonical_id=canonical_id, | |
| actor_user_id=user.user_id, | |
| before_status=record.get("status") or "active", | |
| after_status=updated.get("status") or "active", | |
| before_token_min_iat=before_iat, | |
| after_token_min_iat=ts, | |
| ) | |
| ts = int(time.time()) | |
| before_iat = record.get("token_min_iat") or 0 | |
| updated = await store.bump_token_min_iat(canonical_id, ts) | |
| if updated is None: | |
| return JSONResponse({"error": "not found"}, status_code=404) | |
| after_iat = updated.get("token_min_iat") or ts | |
| await _audit_governance( | |
| request, | |
| action="rotate-tokens", | |
| canonical_id=canonical_id, | |
| actor_user_id=user.user_id, | |
| before_status=record.get("status") or "active", | |
| after_status=updated.get("status") or "active", | |
| before_token_min_iat=before_iat, | |
| after_token_min_iat=after_iat, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/routes/agent_registry.py` around lines 771 - 786, Update the
token-rotation flow around bump_token_min_iat and _audit_governance to use the
persisted token_min_iat from updated for after_token_min_iat, and ensure the
response body reports that same value instead of the requested ts.
…#2205) Add token_min_iat INTEGER column (default 0) to agent_registry so tokens can be invalidated per-identity without revoking the whole identity. - migration _migration_v6_add_token_min_iat: idempotent ALTER TABLE with default 0 so the migration cannot lock the fleet out - AgentRegistryStore.bump_token_min_iat(canonical_id, ts): persist cutoff - Auth path (_verify_agent_scope): after the status check, reject tokens whose iat claim is < the record's token_min_iat with 401 'token superseded' - POST /api/agents/registry/{id}/rotate-tokens: session-owner/admin route that bumps the cutoff and writes a forensic audit-log entry - Tests: store-level (bump sets cutoff, default zero, nonexistent nil), auth-path (old token rejected, new token passes, default-zero safe), route (admin rotates, owner rotates, nonexistent 404)
- bump_token_min_iat: use MAX(token_min_iat, ?) so the cutoff is monotonically non-decreasing — a stale/lower timestamp cannot silently un-supersede tokens - rotate-tokens route: handle None from bump_token_min_iat (race condition) with 404 instead of AttributeError on updated.get() - audit entry: capture before_token_min_iat and after_token_min_iat in the governance audit payload so rotation values are traceable - tests: remove unused token bindings in route-level tests
e614763 to
e6fa1e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tinyagentos/routes/agent_registry.py (1)
279-281: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueChain the exception.
Ruff reports B904. Add
from excso the originalValueErrorstays in the traceback.♻️ Proposed fix
except ValueError as exc: # The reserved-prefix guard rejected the name. Client error, not 500. - raise HTTPException(status_code=422, detail=str(exc)) + raise HTTPException(status_code=422, detail=str(exc)) from excAs per static analysis: "Within an
exceptclause, raise exceptions withraise ... from errorraise ... from None".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/agent_registry.py` around lines 279 - 281, Update the HTTPException raise in the ValueError handler to explicitly chain it from the caught exc, preserving the original ValueError traceback while keeping the existing 422 status and detail.Source: Linters/SAST tools
tests/test_token_rotation.py (2)
304-306: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe assertion does not check what the comment states.
The comment says
token_min_iat is 0, but the assertion only checks that the record exists. Assert the field directly.💚 Proposed fix
- assert (await registry.get(cid)) is not None # token_min_iat is 0 + assert (await registry.get(cid))["token_min_iat"] == 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_token_rotation.py` around lines 304 - 306, Update the assertion after minting old_token to inspect the registry record’s token_min_iat field directly and verify it equals 0, rather than only asserting that registry.get(cid) returns a record.
22-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
client, then add onlyagent_registry/agent_grantsinit.
agent_appduplicates the rootclientfixture fromtests/conftest.py, including admin setup and teardown. Injectclientand add the registry-specific init before yielding it, then remove the copied store init/close block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_token_rotation.py` around lines 22 - 236, Refactor the agent_app fixture to accept the existing client fixture, initialize only app.state.agent_registry and app.state.agent_grants when needed, and yield client directly. Remove the duplicated admin-session setup, store initialization, and teardown logic from agent_app while preserving the registry-specific initialization before yielding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_token_rotation.py`:
- Around line 254-273: Update the _register_and_mint helper so its
registry.register call passes the received user_id value, ensuring the persisted
agent record owner matches the user embedded in the minted token while
preserving the existing default behavior.
---
Nitpick comments:
In `@tests/test_token_rotation.py`:
- Around line 304-306: Update the assertion after minting old_token to inspect
the registry record’s token_min_iat field directly and verify it equals 0,
rather than only asserting that registry.get(cid) returns a record.
- Around line 22-236: Refactor the agent_app fixture to accept the existing
client fixture, initialize only app.state.agent_registry and
app.state.agent_grants when needed, and yield client directly. Remove the
duplicated admin-session setup, store initialization, and teardown logic from
agent_app while preserving the registry-specific initialization before yielding.
In `@tinyagentos/routes/agent_registry.py`:
- Around line 279-281: Update the HTTPException raise in the ValueError handler
to explicitly chain it from the caught exc, preserving the original ValueError
traceback while keeping the existing 422 status and detail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3319af71-b23f-46aa-84cc-bc4bb028b3d0
📒 Files selected for processing (5)
tests/test_agent_registry_store.pytests/test_token_rotation.pytinyagentos/agent_registry_store.pytinyagentos/agent_token_auth.pytinyagentos/routes/agent_registry.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tinyagentos/agent_token_auth.py
- tests/test_agent_registry_store.py
- tinyagentos/agent_registry_store.py
| async def _register_and_mint(app, *, user_id="u", scopes=("a2a_receive",)): | ||
| """Register an active agent, add grants, and mint a signed JWT. | ||
|
|
||
| Returns (canonical_id, token). | ||
| """ | ||
| registry = app.state.agent_registry | ||
| grants = app.state.agent_grants | ||
| priv, _pub = app.state.agent_registry_keypair | ||
| rec = await registry.register( | ||
| framework="test", | ||
| display_name="TestAgent", | ||
| origin="external-selfjoin", | ||
| handle="@test", | ||
| ) | ||
| cid = rec["canonical_id"] | ||
| await registry.set_status(cid, "active") | ||
| for scope in scopes: | ||
| await grants.add_grant(cid, scope) | ||
| token = mint_registry_token(cid, priv, user_id=user_id, framework="test") | ||
| return cid, token |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
user_id is not persisted on the registry record.
_register_and_mint accepts user_id but passes it only to mint_registry_token. The call to registry.register(...) omits user_id=, so the stored row keeps the default owner value. Any test that relies on record ownership (for example the owner branch of require_owner_or_admin) cannot work with this helper.
🐛 Proposed fix
rec = await registry.register(
framework="test",
display_name="TestAgent",
+ user_id=user_id,
origin="external-selfjoin",
handle="`@test`",
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def _register_and_mint(app, *, user_id="u", scopes=("a2a_receive",)): | |
| """Register an active agent, add grants, and mint a signed JWT. | |
| Returns (canonical_id, token). | |
| """ | |
| registry = app.state.agent_registry | |
| grants = app.state.agent_grants | |
| priv, _pub = app.state.agent_registry_keypair | |
| rec = await registry.register( | |
| framework="test", | |
| display_name="TestAgent", | |
| origin="external-selfjoin", | |
| handle="@test", | |
| ) | |
| cid = rec["canonical_id"] | |
| await registry.set_status(cid, "active") | |
| for scope in scopes: | |
| await grants.add_grant(cid, scope) | |
| token = mint_registry_token(cid, priv, user_id=user_id, framework="test") | |
| return cid, token | |
| async def _register_and_mint(app, *, user_id="u", scopes=("a2a_receive",)): | |
| """Register an active agent, add grants, and mint a signed JWT. | |
| Returns (canonical_id, token). | |
| """ | |
| registry = app.state.agent_registry | |
| grants = app.state.agent_grants | |
| priv, _pub = app.state.agent_registry_keypair | |
| rec = await registry.register( | |
| framework="test", | |
| display_name="TestAgent", | |
| user_id=user_id, | |
| origin="external-selfjoin", | |
| handle="`@test`", | |
| ) | |
| cid = rec["canonical_id"] | |
| await registry.set_status(cid, "active") | |
| for scope in scopes: | |
| await grants.add_grant(cid, scope) | |
| token = mint_registry_token(cid, priv, user_id=user_id, framework="test") | |
| return cid, token |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_token_rotation.py` around lines 254 - 273, Update the
_register_and_mint helper so its registry.register call passes the received
user_id value, ensuring the persisted agent record owner matches the user
embedded in the minted token while preserving the existing default behavior.
Docs-Reviewed: retrigger CI after author identity fix; no API surface changes
2 participants
Closes #2205.
What
Add
token_min_iatINTEGER column (default 0) toagent_registryso registry JWT tokens can be invalidated per-identity without revoking the whole identity.Changes
ALTER TABLEwith default 0 — existing rows keep their tokens valid so the migration cannot lock the fleet outAgentRegistryStore.bump_token_min_iat(canonical_id, ts)— persists the cutoff_verify_agent_scope, reject tokens whoseiatclaim is strictly less than the record'stoken_min_iatwith 401"token superseded"POST /api/agents/registry/{id}/rotate-tokens— session-owner/admin route that bumps the cutoff and writes a forensic audit-log entry (action:rotate-tokens)Tests (9 new, all passing)
Store-level: bump sets cutoff, default zero on registration, nonexistent returns None
Auth-path: old token rejected after bump (401), new token passes after bump, default-zero keeps existing tokens valid
Route: admin rotates, owner rotates, nonexistent returns 404
Rotation flow
No existing test regressions — 138 store + 43 route + 11 auth = all green.
Summary by CodeRabbit
New Features
Tests